Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(ci): test pr-1929 #1952

Closed
wants to merge 65 commits into from
Closed

test(ci): test pr-1929 #1952

wants to merge 65 commits into from

Conversation

aajimal
Copy link
Contributor

@aajimal aajimal commented Feb 6, 2025

Summary

Brief summary of the changes made, ie "what?"

Background

Brief background on why these changes were made, ie "why?"

Changes

  • List changes which were made.

Testing

How are these changes tested?

Changelogs

Ensure all relevant changelog files are updated as necessary. See
keepachangelog for change
categories. Replace this text with e.g. "Changelogs updated." or "No updates
required." to acknowledge changelogs have been considered.

Metrics

  • List out metrics added by PR, delete section if none.

Breaking Changelist

  • Bulleted list of breaking changes, any notes on migration. Delete section if none.

Related Issues

Link any issues that are related, prefer full GitHub links.

closes

noot and others added 30 commits November 5, 2024 19:15
…1236)

## Summary
integrate skip's [connect](https://github.com/skip-mev/connect)
(formerly named `slinky`) oracle service into astria.

at a high level, connect consists of an oracle sidecar program, which
interacts with a validator node to provide price data, and various
cosmos-sdk modules.

since astria isn't cosmos, the relevant cosmos modules (x/marketmap and
x/oracle) were essentially ported into the `connect` module of the
sequencer app, which consists of two components, `market_map` and
`oracle`.

the sequencer app was updated to talk to the sidecar during the
`extend_vote` phase of consensus to gather prices to put into a vote
extension.

the vote extension validation logic, proposal logic, and finalization
logic were also ported from connect.

## Background
we want oracle data to be available to rollups (and maybe on the
sequencer itself too?)

## Changes
* import relevant protos from connect and create native rust types for
them
* update the sequencer genesis state to contain `market_map` and
`oracle` values
* implement the `market_map` component for the sequencer
* update the sequencer grpc service to support the market_map grpc
service, which is required by the oracle sidecar to retrieve the market
map from the chain
* implement the `oracle` component for the sequencer and the query
service for this component
* implement `extend_vote` logic which gets the prices from the sidecar
and turns them into a vote extension
* implement `verify_vote_extension` logic which performs basic
validation on a vote extension during the consensus phase
* implement `prepare_proposal` logic which gathers the vote extensions
from the previous block, prunes any invalid votes, and performs
additional validation to create a valid set of VEs
* implement `process_proposal` logic which validates the set of VEs
proposed, checking signatures and that the voting power is >2/3 amongst
other things
* implement `finalize_block` logic which writes the updated prices to
state based on the committed vote extensions. skip uses stake-weighted
median to calculate the final price, but we don't have stake-weighting
yet, so i just took the median.
* TODO: implement the connect cosmos `Msg` types as sequencer actions
(follow-up)
*
https://github.com/skip-mev/slinky/blob/158cde8a4b774ac4eec5c6d1a2c16de6a8c6abb5/proto/slinky/oracle/v1/tx.proto
*
https://github.com/skip-mev/slinky/blob/158cde8a4b774ac4eec5c6d1a2c16de6a8c6abb5/proto/slinky/marketmap/v1/tx.proto
* TODO: update `SequencerBlockHeader` to contain the extended commit
info + a proof for it (also follow-up)
* TODO: implement the `DeltaCurrencyPairStrategy` - right now only the
`DefaultCurrencyPairStrategy` is implemented. can also do in follow-up

## Testing
TODO: run this on a multi-validator network also

clone connect: https://github.com/skip-mev/connect/tree/main

install go 1.22

build and run connect:

```sh
make build
go run scripts/genesis.go --use-coingecko=true --temp-file=markets.json
./build/connect --market-config-path markets.json --port 8081
```

checkout `noot/slinky` branch of astria

run sequencer app and `ASTRIA_SEQUENCER_NO_ORACLE=false` in `.env`:

```sh
rm -rf /tmp/astria_db
rm -rf ~/.cometbft
just run
just run-cometbft
```

should see a sequencer log like:

```sh
astria_sequencer::sequencer: oracle sidecar is reachable
```

should see a connect log like:


```sh
{"level":"info","ts":"2024-07-02T14:33:46.318-0400","caller":"marketmap/fetcher.go:147","msg":"successfully fetched market map data from module; checking if market map has changed","pid":727051,"process":"oracle","fetcher":"marketmap_api"}
```

then, when blocks are made, should see logs like the following for each
block:


```
2024-07-05T02:49:21.254163Z DEBUG handle_request:handle_process_proposal: astria_sequencer::service::consensus: proposal processed height=28 time=2024-07-05T02:49:19.143352683Z tx_count=3 proposer=7BE21CDEB6FDCC9299A51F44C6B390EA990E88CD hash=7rmdhOsaW2a0NCZUwSE5yqt2AVR3cOPgGGb4Bb0kpRM= next_validators_hash=F6N7YDQZKfXQld95iV0AmQKNa8DiAxrDnTAcn323QSU=
2024-07-05T02:49:21.310218Z DEBUG handle_request:handle_extend_vote:App::extend_vote: astria_sequencer::app::vote_extension: got prices from oracle sidecar; transforming prices prices_count=118
2024-07-05T02:49:21.323262Z DEBUG handle_request:handle_extend_vote:App::extend_vote: astria_sequencer::app::vote_extension: transformed price for inclusion in vote extension currency_pair="BTC/USD" id=0 price=5683583007
2024-07-05T02:49:21.326070Z DEBUG handle_request:handle_extend_vote:App::extend_vote: astria_sequencer::app::vote_extension: transformed price for inclusion in vote extension currency_pair="ETH/USD" id=1 price=3055069469
2024-07-05T02:49:21.384266Z DEBUG handle_request:finalize_block:App::finalize_block: astria_sequencer::app::vote_extension: applied price from vote extension currency_pair="BTC/USD" price=5683583007 hash=EEB99D84EB1A5B66B4342654C12139CAAB7601547770E3E01866F805BD24A513 height=28 time=2024-07-05T02:49:19.143352683Z proposer=7BE21CDEB6FDCC9299A51F44C6B390EA990E88CD
2024-07-05T02:49:21.384553Z DEBUG handle_request:finalize_block:App::finalize_block: astria_sequencer::app::vote_extension: applied price from vote extension currency_pair="ETH/USD" price=3055069469 hash=EEB99D84EB1A5B66B4342654C12139CAAB7601547770E3E01866F805BD24A513 height=28 time=2024-07-05T02:49:19.143352683Z proposer=7BE21CDEB6FDCC9299A51F44C6B390EA990E88CD
```

## Breaking Changelist
* the PR adds a new proposer-added transaction at
the start of the block only if vote extensions are enabled. then, there will be 3 special "txs" expected when
before there were only 2. however if vote extensions are disabled, this won't make a difference.
* the genesis was updated with a new optional field `connect`, however
as this field is optional, it is non-breaking with existing networks.
* if the `connect` genesis field is set, the sequencer state will
change, as the genesis state changes and new values are stored in state.
however this does not affect syncing existing networks, as the genesis
of the existing network can be used as-is.
* additionally, vote extension participation is optional - if <=2/3
validators participate, blocks are still finalized, just no new oracle
data will be published.

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
## Summary
This mainly brings the new oracle code in line with storage conventions
formed after the initial cut of the PR.

## Background
We want a consistent approach to storage across all components. As well
as using a single serialization format (Borsh), this will allow us to
iterate on the storage design in the near future.

## Changes
- Added `storage` submodules to the two sequencer modules
`connect::market_map` and `connect::oracle`. These provide the keys and
values which are used to store the relevant data.
- Added snapshot tests and other unit tests for the keys.
- Added put/get tests for the extension traits, which revealed bugs in
both the streaming getters of the oracle.
- Removed unneeded serde trait derives from several core types.
- Renamed `connect::marketmap` in the sequencer to `connect::market_map`

## Testing
Added several unit tests.

## Changelogs
We should update changelogs once the feature is ready to be merged to
`main`. Not done in this PR.

## Breaking Changelist
- This breaks the state snapshot tests due to changing how the oracle
data is serialized in storage.

---------

Co-authored-by: elizabeth <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
…sidecar (#1788)

## Summary
This changes sequencer initialization to be blocked while attempting to
connect to the oracle sidecar (if configured to use the sidecar), and to
fail if repeated connection attempts fail.

## Background
This was a todo in the code.

## Changes
- Added blocking retry logic around creating a connected oracle client.

## Testing
Added a unit test to confirm the process waits for the expected
duration. We could probably do with adding gRPC mock tests for the
sidecar, but I think that's outside the scope of this PR.

## Changelogs
No updates required as this is (effectively) a feature branch which will
have all relevant changelogs updated as one of the final steps of
completing the feature.

## Related Issues
Closes #1787

---------

Co-authored-by: elizabeth <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
## Summary
make extended commit info third injected tx, and update
`SequencerBlock::try_from_block_info_and_data` to handle whether the
extended commit info is in the block or not.

## Background
it makes more sense to append to the existing list of injected txs which
are the two commitments at the start of the block.

## Changes
- make extended commit info the third injected tx instead of the first
- update `SequencerBlock::try_from_block_info_and_data` to handle
whether the extended commit info is in the block or not
- cleanup of related unit tests

## Testing
unit tests

## Breaking Changelist
- unbreaks `SequencerBlock::try_from_block_info_and_data`
…ncerBlock` (#1817)

## Summary
put extended commit info and merkle proof of inclusion into
`SequencerBlock`.

## Background
required for propagating oracle data inside the extended commit info to
rollups.

## Changes
- put extended commit info and merkle proof of inclusion into
`SequencerBlock` as optional types. if there is no extended commit info
for that block (ie. vote extensions are disabled) then they remain
unset. this is for backwards-compatibility
- also put put extended commit info and proof into celestia
`SubmittedMetadata` (so it is written to DA)

## Testing
unit tests

## Breaking Changelist
- `SequencerBlock` proto is updated with new fields; however, if these
fields are unset, the native `SequencerBlock` sets these to `None`, so
this branch should be able to sync with networks without vote extensions
enabled
- same for the `SubmittedMetadata` proto

## Related Issues

closes  #1764
…extended commit info (#1832)

## Summary
the price data posted by validators is of the form currency pair ID ->
price, however when this is sent to a rollup there is then no way to
determine the ID -> actual currency pair without referencing the
sequencer state. this is non ideal as we don't want to have to perform
lookups using a sequencer node for the oracle data.

## Background
see above

## Changes
- create `ExtendedCommitInfoWithCurrencyPairMapping` type which contains
`ExtendedCommitInfo` (as previous) as well as the ID->currency pair
mapping for the price data inside that `ExtendedCommitInfo`
- the proposer fills this mapping using the current sequencer state
(before tx execution)
- encoded `ExtendedCommitInfoWithCurrencyPairMapping` is then used as
the third injected tx in the block, whereas before it was just
`ExtendedCommitInfo`
- the other validators then verify this mapping using the current
sequencer state (before tx execution) in `process_proposal`


## Testing
unit tests + CI tests

## Related Issues
required for #1818
## Summary
Increase test coverage of the `app::vote_extensions` module and fixes
issues discovered while implementing these tests.

## Background
This was work spun out of previous PRs to this feature branch to allow
parallelizing efforts to complete the feature.

## Changes
- Provided comprehensive unit test coverage of the
`app::vote_extensions` module.
- Fixed a handful of issues uncovered.

## Testing
These are tests.

## Changelogs
No updates required - the changelogs will be updated right before the
feature merges to `main`.

---------

Co-authored-by: elizabeth <[email protected]>
## Summary
This is a simple merge of `main`. It is based on top of the commits in
`noot/more-tests` which forms PR #1836, so this is blocked by that PR.

## Background
We want to keep the feature branch as in sync with `main` as possible.

## Changes
- Merged `main`. Surprisingly few conflicts, and all trivial: only three
in the following files:
    - crates/astria-core/Cargo.toml
    - crates/astria-core/src/generated/mod.rs
    - crates/astria-core/src/lib.rs

## Testing
No new tests added.

## Changelogs
No updates required.

---------

Co-authored-by: Itamar Reif <[email protected]>
Co-authored-by: quasystaty <[email protected]>
Co-authored-by: Jordan Oroshiba <[email protected]>
Co-authored-by: elizabeth <[email protected]>
Co-authored-by: Ethan Oroshiba <[email protected]>
Co-authored-by: noot <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
…ution API (#1840)

## Summary
update conductor to pass oracle data to rollup via execution API.

## Background
this is the final change needed within the astria monorepo for the
oracle integration, as the oracle data is now fully passed up the stack.
handling the oracle data is now left up to each rollup.

## Changes
- add a new `OracleData` variant to the `RollupData` enum, which
contains a `Vec<Price>`, where each `Price` contains the relevant oracle
data for a currency pair for that block
- update the injected tx type
`ExtendedCommitInfoWithCurrencyPairMapping` such that the id->currency
pair mapping also contains the decimals for that currency pair's price;
this is necessary information that I overlooked in the previous PR
creating that type
- add the extended commit info + proof to `FilteredSequencerBlock`
- update `SequencerBlock`, `FilteredSequencerBlock` and
`SubmittedMetadata` constructors such that the extended commit info
bytes must be a valid encoded protobuf
`ExtendedCommitInfoWithCurrencyPairMapping`
- move functionality for calculating the aggregated prices from the
extended commit info from sequencer to core so that conductor can also
use it
- update conductor's executor to calculate the aggregated price data and
pass it as `RollupData::OracleData` to the rollup via the execution API

## Testing
unit tests

## Related Issues

closes  #1818
## Summary
update `normalize_by_pair` to be optional and fix default market map
genesis.

## Background
connect sidecar can now run without specifying a markets.json, can now
just get the market map from the sequencer.

## Changes
- update `normalize_by_pair` to be optional as it should be optional
-  fix default market map genesis

## Testing
ran connect sidecar locally; change sequencer gRPC port to 9090 and run:
`./build/connect --port 8081`

run sequencer+cometbft as normal, should see sidecar logs like:

```
{"level":"info","ts":"2024-12-04T15:38:30.423-0500","caller":"marketmap/fetcher.go:128","msg":"successfully fetched market map data from module; checking if market map has changed","pid":8547,"process":"oracle","fetcher":"marketmap_api"}
```
## Summary
This is a simple merge of `main`.

## Background
We want to keep the feature branch as in sync with `main` as possible.

## Changes
- Merged `main`. Conflicts in the following files, but all relatively
trivial:
  - crates/astria-cli/src/sequencer/bridge_account.rs
  - crates/astria-cli/src/sequencer/bridge_sudo_change.rs
  - crates/astria-cli/src/sequencer/fee_assets.rs
  - crates/astria-cli/src/sequencer/sudo/ibc_sudo_change.rs
  - crates/astria-core/CHANGELOG.md
  - crates/astria-core/Cargo.toml
  - crates/astria-core/src/generated/mod.rs
  - crates/astria-core/src/lib.rs
  - crates/astria-core/src/primitive/v1/mod.rs
  - crates/astria-core/src/protocol/genesis/v1.rs
  - crates/astria-core/src/sequencerblock/v1/block.rs
  - crates/astria-sequencer-utils/src/genesis_example.rs
  - crates/astria-sequencer/CHANGELOG.md
  - crates/astria-sequencer/src/app/mod.rs
  - crates/astria-sequencer/src/bridge/bridge_lock_action.rs
  - crates/astria-sequencer/src/fees/state_ext.rs
  - crates/astria-sequencer/src/grpc/storage/keys.rs
  - crates/astria-sequencer/src/sequencer.rs
  - justfile
  - tools/protobuf-compiler/src/main.rs

## Testing
No new tests added.

## Changelogs
No updates required.

---------

Co-authored-by: Itamar Reif <[email protected]>
Co-authored-by: quasystaty <[email protected]>
Co-authored-by: Jordan Oroshiba <[email protected]>
Co-authored-by: Ethan Oroshiba <[email protected]>
Co-authored-by: noot <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
Co-authored-by: Josh Bowen <[email protected]>
## Summary
merge main into feat/oracle.

## Changes
files with conflicts:
```
        both modified:   Cargo.lock
        both modified:   crates/astria-core/CHANGELOG.md
        both modified:   crates/astria-core/Cargo.toml
        both modified:   crates/astria-core/src/generated/mod.rs
        both modified:   crates/astria-core/src/lib.rs
        both modified:   crates/astria-core/src/primitive/v1/mod.rs
        both modified:   crates/astria-core/src/protocol/genesis/v1.rs
        both modified:   crates/astria-core/src/sequencerblock/v1/block.rs
        both modified:   crates/astria-sequencer-utils/Cargo.toml
        both modified:   crates/astria-sequencer-utils/src/genesis_example.rs
        both modified:   crates/astria-sequencer/src/action_handler/impls/bridge_sudo_change.rs
        both added:      crates/astria-sequencer/src/action_handler/impls/fee_change.rs
        both added:      crates/astria-sequencer/src/action_handler/impls/transaction.rs
        both modified:   crates/astria-sequencer/src/app/mod.rs
        both modified:   crates/astria-sequencer/src/fees/mod.rs
        both modified:   crates/astria-sequencer/src/fees/query.rs
        both modified:   crates/astria-sequencer/src/grpc/storage/keys.rs
        both modified:   crates/astria-sequencer/src/sequencer.rs
        both modified:   crates/astria-sequencer/src/service/info/mod.rs
        both modified:   tools/protobuf-compiler/src/main.rs
```

## Testing
existing tests
noot and others added 7 commits January 20, 2025 11:53
## Summary
update geth charts for oracle genesis values.

## Background
geth charts needed to be updated for the oracle integration; see
astriaorg/astria-geth#62

## Changes
- add `astriaOracleCallerAddress` and `astriaOracleContractAddress`
config values
- add oracle contract bytecode to genesis alloc at
`astriaOracleContractAddress`
- update dev tags for geth and sequencer

## Testing
ran it end-to-end with the usual dev cluster instructions and queried
the oracle contract for price updates via the instructions here:
https://github.com/astriaorg/astria-oracle-contracts/tree/main?tab=readme-ov-file#query-contract
## Summary
This is a simple merge of `main`.

## Background
We want to keep the feature branch as in sync with `main` as possible.

## Changes
- Merged `main`. Conflicts in the following files, but all relatively
trivial:
  - charts/evm-stack/Chart.lock
  - charts/evm-stack/Chart.yaml
  - crates/astria-conductor/src/celestia/mod.rs
  - crates/astria-core/CHANGELOG.md
- crates/astria-core/src/sequencerblock/v1/block.rs (there were quite a
few in here)
  - crates/astria-sequencer/src/sequencer.rs
  - crates/astria-sequencer/src/service/consensus.rs
- Also had to fix a few clippy lints since the Rust version has been
updated, again all trivial fixes.

## Testing
No new tests added.

## Changelogs
No updates required.
## Summary
Added select market map actions
[here](https://github.com/skip-mev/connect/blob/main/proto/connect/marketmap/v2/tx.proto)
to the sequencer.

## Background
These actions were needed for support for keeping a `MarketMap` in state
and making changes to it as part of the oracle feature. They follow
Skip's logic and apply it to our state machine.

#### `ChangeMarkets`
Takes a list of markets and either creates, updates, or removes them
depending on its variant. Must be signed by an address included in the
market map params' `market_authorities`.
- **Create:** Creates the markets in the market map. If no market map is
found, one will be created. If any of the markets to create already
exist, this action will err.
- **Update:** Updates the markets in the market map, matching based on
`Ticker.currency_pair`). If no market map is found, or any market is
missing a counterpart in the map, this action will err.
- **Remove:** Removes the markets from the market map. If a market is
not found in the map, it will be ignored.

#### `UpdateParams`
Updates the market map `Params`, which contains the market authority
addresses as well as an admin address. This will execute whether there
are params in the state already or not. Must be signed by the sequencer
network authority sudo address.

## Changes
- Added `ChangeMarkets` and `UpdateParams` to `PriceFeed` action in
proto, core, and sequencer.

## Testing
- Added fairly exhaustive unit tests to each action's implementation
module.
- Added app execution test for each action for more high-level testing.
- Added each action to all action snapshot.

## Breaking Changelist
- Added new actions which break app hash due to adding new information
to state.
- Added `ALICE_ADDRESS` to market authorities in testing `genesis`,
breaking two more snapshots.
## Summary
This is a small PR to address a couple of minor omissions: implementing
a query endpoint, and removing a needless RPC call.

## Background
These are likely just small omissions from previous work/discussions.

## Changes
- Updated `MarketMapQueryService::market` to not always return a "not
implemented" error response.
- Changed initial connection attempts to oracle sidecar to not actually
send a request.

## Testing
Manually tested both changes locally. The former would ideally have unit
tests, but this entire module is not unit tested. The latter is harder
to create a meaningful unit test for, but new smoke tests using the
sidecar will make it apparent if this function is broken.

## Changelogs
No updates required.
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate sequencer-relayer pertaining to the astria-sequencer-relayer crate cd labels Feb 6, 2025
@aajimal aajimal added preview docker-build used to trigger docker builds on PRs labels Feb 6, 2025
@github-actions github-actions bot added the ci issues that are related to ci and github workflows label Feb 6, 2025
@aajimal aajimal added preview and removed preview conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate docker-build used to trigger docker builds on PRs cd override-freeze sequencer-relayer pertaining to the astria-sequencer-relayer crate labels Feb 6, 2025
@aajimal aajimal closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci issues that are related to ci and github workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants